fix(im): accept absolute paths for --image/--file flags#905
Conversation
The --image and --file flags on im +messages-send and +messages-reply rejected absolute paths with a validation error. This is inconvenient for AI agents and scripted callers that construct absolute paths. Changes: - Added SafeAbsoluteInputPath() to validate package for absolute paths that still checks control characters and resolves symlinks - Updated validateMediaFlagPath() to accept absolute paths after safety validation - Added openMediaFile/statMediaFile helpers that use os.Open/os.Stat for absolute paths and FileIO for relative paths - Updated help text to indicate absolute paths are accepted Fixes larksuite#872
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds safe absolute-path validation and path-type-aware file I/O so IM ChangesAbsolute Path Support for Media Flags
Sequence DiagramsequenceDiagram
participant User
participant CLI as im +messages-send
participant Validator as validateMediaFlagPath
participant SafeAbs as SafeAbsoluteInputPath
participant FileIO as openMediaFile/statMediaFile
participant OS as os / runtime.FileIO
User->>CLI: --image /tmp/image.png
CLI->>Validator: image value
Validator->>SafeAbs: /tmp/image.png
SafeAbs->>SafeAbs: reject control chars, resolve symlinks
SafeAbs-->>Validator: validated path or error
Validator-->>CLI: valid
CLI->>FileIO: open/stat /tmp/image.png
FileIO->>OS: use os.Open/os.Stat (absolute)
OS-->>FileIO: file handle/info
FileIO-->>CLI: ready for upload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/im/helpers.go (1)
571-594:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDuration parsing will fail for absolute audio/video paths.
parseMediaDurationcallsruntime.FileIO().Stat()andruntime.FileIO().Open()(lines 575, 579), which reject absolute paths outside the working directory. For absolute audio/video files, duration parsing will fail silently (returning ""), causing uploads to succeed but omit the duration field.🔧 Suggested fix
Refactor
parseMediaDurationto use the dual-mode helpers:func parseMediaDuration(runtime *common.RuntimeContext, filePath, fileType string) string { if fileType != "opus" && fileType != "mp4" { return "" } - info, err := runtime.FileIO().Stat(filePath) + info, err := statMediaFile(runtime, filePath) if err != nil || info.Size() == 0 { return "" } - f, err := runtime.FileIO().Open(filePath) + f, err := openMediaFile(runtime, filePath) if err != nil { return "" } + defer f.Close() var ms int64 if fileType == "opus" { - ms = readOggDuration(f, info.Size()) + ms = readOggDuration(f.(fileio.File), info.Size()) } else { - ms = readMp4Duration(f, info.Size()) + ms = readMp4Duration(f.(fileio.File), info.Size()) } if ms <= 0 { return "" } return strconv.FormatInt(ms, 10) }Note: You'll need to update
readOggDurationandreadMp4Durationto acceptio.ReaderAtinstead offileio.File, or add type assertions as shown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/helpers.go` around lines 571 - 594, parseMediaDuration currently fails for absolute paths because it uses runtime.FileIO().Stat() and Open() which reject absolute locations; refactor parseMediaDuration to use the dual-mode file helpers (open a reader that supports both filesystem and external absolute URLs) instead of directly calling runtime.FileIO().Stat/Open, and update readOggDuration and readMp4Duration to accept a generic io.ReaderAt (or perform a safe type assertion on the returned reader) so you can pass the reader returned by the dual-mode helper; ensure ms calculation and error handling remain the same in parseMediaDuration and return the formatted ms when >0.
🧹 Nitpick comments (1)
shortcuts/im/helpers.go (1)
1323-1334: ⚡ Quick winFragile type assertion could panic.
Line 1333 asserts
fi.(os.FileInfo)without checking. If a test uses a custom FileIO implementation that returns a fileio.FileInfo not implementing os.FileInfo, this will panic.♻️ Simpler signature
The callers only need
.Size(), which is available onfileio.FileInfo. Change the return type:-func statMediaFile(runtime *common.RuntimeContext, filePath string) (os.FileInfo, error) { +func statMediaFile(runtime *common.RuntimeContext, filePath string) (fileio.FileInfo, error) { if filepath.IsAbs(filePath) { return os.Stat(filePath) } fi, err := runtime.FileIO().Stat(filePath) if err != nil { return nil, err } - return fi.(os.FileInfo), nil + return fi, nil }This avoids the assertion and works correctly with both real and mock FileIO.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/helpers.go` around lines 1323 - 1334, statMediaFile currently returns os.FileInfo and does an unchecked type assertion fi.(os.FileInfo) which can panic for custom FileIO implementations; change statMediaFile to return fileio.FileInfo (or the common interface used by callers) and update its signature on callers to use .Size(); for absolute paths adapt the os.Stat result to fileio.FileInfo (add a small adapter type or converter that wraps an os.FileInfo and implements fileio.FileInfo) and for the runtime.Path case return the runtime.FileIO().Stat(filePath) value directly without asserting to os.FileInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/helpers.go`:
- Around line 1313-1334: Update the comments for openMediaFile and statMediaFile
to clearly state these helpers require paths to be pre-validated (e.g., by
validateMediaFlagPath) before being passed in; explicitly mention that absolute
paths are opened/sta'd with os.Open/os.Stat without further checks and that
callers must ensure the path is safe and validated to avoid security risks.
Reference the functions openMediaFile and statMediaFile and the validator
validateMediaFlagPath in the comment so future maintainers see the required
precondition and the reason (absolute paths bypass runtime.FileIO protections).
---
Outside diff comments:
In `@shortcuts/im/helpers.go`:
- Around line 571-594: parseMediaDuration currently fails for absolute paths
because it uses runtime.FileIO().Stat() and Open() which reject absolute
locations; refactor parseMediaDuration to use the dual-mode file helpers (open a
reader that supports both filesystem and external absolute URLs) instead of
directly calling runtime.FileIO().Stat/Open, and update readOggDuration and
readMp4Duration to accept a generic io.ReaderAt (or perform a safe type
assertion on the returned reader) so you can pass the reader returned by the
dual-mode helper; ensure ms calculation and error handling remain the same in
parseMediaDuration and return the formatted ms when >0.
---
Nitpick comments:
In `@shortcuts/im/helpers.go`:
- Around line 1323-1334: statMediaFile currently returns os.FileInfo and does an
unchecked type assertion fi.(os.FileInfo) which can panic for custom FileIO
implementations; change statMediaFile to return fileio.FileInfo (or the common
interface used by callers) and update its signature on callers to use .Size();
for absolute paths adapt the os.Stat result to fileio.FileInfo (add a small
adapter type or converter that wraps an os.FileInfo and implements
fileio.FileInfo) and for the runtime.Path case return the
runtime.FileIO().Stat(filePath) value directly without asserting to os.FileInfo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1ccb1ba-6e7a-4360-8311-d95cd27a9ce3
📒 Files selected for processing (5)
internal/validate/path.gointernal/vfs/localfileio/path.goshortcuts/im/helpers.goshortcuts/im/im_messages_reply.goshortcuts/im/im_messages_send.go
Address CodeRabbit review: document that callers must validate paths before calling these helpers, since they use os.Open/os.Stat directly for absolute paths without additional safety checks.
|
@CLAassistant recheck please |
|
@CLAassistant recheck |
1 similar comment
|
@CLAassistant recheck |
Summary
Fixes #872
The
--imageand--fileflags onim +messages-sendandim +messages-replyrejected absolute paths with a validation error. This is inconvenient for AI agents and scripted callers that construct absolute paths.Changes
SafeAbsoluteInputPath()tovalidatepackage — validates absolute paths for safety (control characters, symlink resolution) without restricting to the working directoryvalidateMediaFlagPath()to accept absolute paths after safety validationopenMediaFile/statMediaFilehelpers inhelpers.gothat useos.Open/os.Statfor absolute paths andFileIOfor relative paths--imageand--fileflags to indicate absolute paths are acceptedSecurity
FileIOwhich restricts to the working directoryLocalFileIO.OpenandLocalFileIO.Statmethods are unchanged — the security boundary is only relaxed for explicitly user-provided media flag valuesSummary by CodeRabbit
New Features
Improvements